[6421642][ONNX][Quantization] Fix NVFP4 exporter node ordering#1985
[6421642][ONNX][Quantization] Fix NVFP4 exporter node ordering#1985gcunhase wants to merge 7 commits into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNVFP4 ONNX post-processing now reuses matching input casts and deterministically topologically sorts graph nodes after FP4QDQ replacement and initializer cleanup. CPU tests cover cast reuse, FP4QDQ removal, subgraph captures, and ONNX validation. ChangesNVFP4 ONNX Export
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
303dda2 to
226f0f3
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1985 +/- ##
==========================================
- Coverage 77.38% 77.27% -0.12%
==========================================
Files 524 524
Lines 59263 59323 +60
==========================================
- Hits 45860 45840 -20
- Misses 13403 13483 +80
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
226f0f3 to
d3f7199
Compare
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-5.6-sol) — DM the bot to share feedback.
The fix is straightforward and consistent with existing exporter patterns: GraphSurgeon topologically sorts the graph after the NVFP4 post-processing code appends producer nodes, and the new CPU regression test exercises the real Torch→ONNX→NVFP4 conversion path and validates the result with ONNX checker. Imports are correctly placed, the existing ONNX optional dependency already includes GraphSurgeon, and no licensing changes are introduced.
|
/claude review |
There was a problem hiding this comment.
Claude review passed — no blocking issues found. LGTM
Scope: The PR's authoritative changed-file list is 2 files — modelopt/onnx/export/nvfp4_exporter.py and tests/unit/torch/quantization/test_onnx_export_cpu.py. (Local base drift also surfaced trt_utils.py/test_plugin.py hunks, but those are not part of this PR and were excluded.) Both PR files were reviewed in full.
Findings — CRITICAL: 0, IMPORTANT: 0, SUGGESTION: 0
Assessment (low risk):
- The fix appends a GraphSurgeon
import_onnx → toposort() → export_onnxround-trip at the end ofNVFP4QuantExporter.post_process(), correctly reordering the newly-insertedDequantizeLinear/Castproducer nodes ahead of theTranspose/MatMulconsumers they feed. This matches the established pattern used by the sibling FP8/INT4/INT8 exporters andgraph_utils, andonnx-graphsurgeon>=0.6.1is already a declared dependency (no new import risk). - The sole caller (
_deploy/utils/torch_onnx.py:477) reassignsprocess_model()'s return, so returning a fresh ModelProto fromgs.export_onnx()is safe — nothing relies on in-place identity. - The new CPU regression test drives the real Torch→ONNX→NVFP4 path, asserts
TRT_FP4QDQis present pre-conversion and gone post-conversion, and validates withonnx.checker.check_model(which itself enforces topological ordering) — a real guard for this bug.
60e28c2 to
3ccb0bd
Compare
3ccb0bd to
7c448e1
Compare
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🧹 Nitpick comments (2)
modelopt/onnx/export/nvfp4_exporter.py (2)
467-467: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the ordering contract directly in the regression test.
The supplied test checks FP4QDQ removal and
onnx.checker.check_model, but does not explicitly verify that every in-graph producer precedes its consumer. Add that edge-order assertion, ideally with a branched graph, so this new sorting call cannot silently regress.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelopt/onnx/export/nvfp4_exporter.py` at line 467, Extend the regression test covering the export flow around _topologically_sort_graph_nodes to assert that every in-graph producer node appears before its consumer, preferably using a branched graph. Keep the existing FP4QDQ-removal and onnx.checker.check_model checks, and make the new assertion validate the sorted node order directly.
62-71: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a heap for the ready queue.
pop(0)shifts the remaining list, andready.sort()runs on every iteration. For wide ONNX graphs this can degrade toward quadratic/logarithmic behavior and make export unnecessarily slow. Useheapqkeyed by the original node index to preserve stable ordering withO((V+E) log V)behavior.Proposed change
+import heapq + - ready = [node_index for node_index, dependency in enumerate(dependencies) if not dependency] + ready = [node_index for node_index, dependency in enumerate(dependencies) if not dependency] + heapq.heapify(ready) sorted_nodes: list[onnx.NodeProto] = [] while ready: - node_index = ready.pop(0) + node_index = heapq.heappop(ready) sorted_nodes.append(nodes[node_index]) for dependent_index in sorted(dependents[node_index]): dependencies[dependent_index].remove(node_index) if not dependencies[dependent_index]: - ready.append(dependent_index) - ready.sort() + heapq.heappush(ready, dependent_index)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelopt/onnx/export/nvfp4_exporter.py` around lines 62 - 71, Replace the list-based ready queue in the topological sorting flow with a heapq min-heap keyed by original node indices. Heapify the initial ready indices, use heap push/pop operations when nodes become ready, and remove the per-iteration sorting while preserving stable ascending node ordering and the existing dependency processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modelopt/onnx/export/nvfp4_exporter.py`:
- Line 467: Update _cast_input_dtypes() to cache and reuse Cast outputs keyed by
(input_name, precision_dtype), ensuring shared activations produce only one Cast
node and tensor. Preserve the existing <input>_f16 naming where applicable, and
ensure _topologically_sort_graph_nodes() receives a graph without duplicate
producers.
---
Nitpick comments:
In `@modelopt/onnx/export/nvfp4_exporter.py`:
- Line 467: Extend the regression test covering the export flow around
_topologically_sort_graph_nodes to assert that every in-graph producer node
appears before its consumer, preferably using a branched graph. Keep the
existing FP4QDQ-removal and onnx.checker.check_model checks, and make the new
assertion validate the sorted node order directly.
- Around line 62-71: Replace the list-based ready queue in the topological
sorting flow with a heapq min-heap keyed by original node indices. Heapify the
initial ready indices, use heap push/pop operations when nodes become ready, and
remove the per-iteration sorting while preserving stable ascending node ordering
and the existing dependency processing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5894027d-c666-4213-b109-316bdf9515e4
📒 Files selected for processing (1)
modelopt/onnx/export/nvfp4_exporter.py
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modelopt/onnx/utils.py (1)
69-78: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a heap for the ready queue.
pop(0)plus sorting on every iteration can make wide graphsO(V² log V). Aheapqmin-heap preserves the current stable index ordering while reducing queue maintenance toO((V+E) log V).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelopt/onnx/utils.py` around lines 69 - 78, Replace the ready-list queue in the topological sorting logic with a heapq min-heap, updating initialization, extraction, and insertion accordingly. Preserve ascending node-index ordering and the existing dependency processing in the loop, while removing repeated pop(0) and ready.sort() operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modelopt/onnx/utils.py`:
- Around line 59-67: Update topologically_sort_graph_nodes to account for
tensors captured by nested If/Loop subgraphs when building dependencies, either
by recursively collecting those outer-scope references or by explicitly
rejecting graphs containing unsupported control-flow nodes. Preserve direct
input dependency handling and add a regression test covering a producer and
control-flow consumer connected through an implicit capture.
---
Nitpick comments:
In `@modelopt/onnx/utils.py`:
- Around line 69-78: Replace the ready-list queue in the topological sorting
logic with a heapq min-heap, updating initialization, extraction, and insertion
accordingly. Preserve ascending node-index ordering and the existing dependency
processing in the loop, while removing repeated pop(0) and ready.sort()
operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5d326a8d-4f4d-4e77-bc32-522926e15de9
📒 Files selected for processing (2)
modelopt/onnx/export/nvfp4_exporter.pymodelopt/onnx/utils.py
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-5.6-sol) — DM the bot to share feedback.
Re-review complete. The prior correctness concerns are addressed: shared activation Casts are now cached per (input_name, precision_dtype) with regression coverage, and the protobuf topological sorter accounts for outer-scope tensors captured by nested control-flow subgraphs with a focused test. The exporter regression still exercises the real Torch→ONNX→NVFP4 path and validates the converted graph with ONNX checker. The remaining heap-based ready-queue suggestion is a non-blocking performance nit; the current stable sort is correct. No licensing changes or blocking issues found.
|
/claude review |
There was a problem hiding this comment.
Claude review passed — no blocking issues found. LGTM
Scope: Authoritative PR file list is 3 files — modelopt/onnx/export/nvfp4_exporter.py, modelopt/onnx/utils.py, and tests/unit/torch/quantization/test_onnx_export_cpu.py. All three reviewed in full. (trt_utils.py/test_plugin.py hunks in the local diff are shallow-checkout base drift, not part of this PR — excluded.)
Findings — CRITICAL: 0, IMPORTANT: 0, SUGGESTION: 0
Assessment (low risk):
topologically_sort_graph_nodesimplements Kahn's algorithm correctly: duplicate-producer / self-dependency / cycle guards raise instead of silently misordering; graph inputs & initializers correctly seed the ready set; stable ascending-index ordering is preserved. Outer-scope tensors captured byIf/Loopsubgraphs are folded into the dependency set with correct recursive propagation, and the new regression test exercises the producer-after-consumer capture case.- Shared-activation
Castreuse is keyed by(input_name, precision_dtype); since precision is computed once per graph this dedupes by input, so multiple NVFP4-lowered MatMuls sharing an activation reuse a singleCast. The_f16/_bf16suffix is an internal tensor name only — nothing downstream string-matches these names, so it is behavior-safe. - The sort runs in place after initializer cleanup and
post_processreturns the same ModelProto; no mode registration,modelopt_stateschema, or public export-config surface is touched. Pure ONNX graph-ordering bug fix. - The only open reviewer item is CodeRabbit's non-blocking
heapqmicro-optimization; export is not a hot path and the current stable sort is correct, so it does not block.
Ensure NVFP4 ONNX post-processing returns graphs with producers before consumers after TRT_FP4QDQ lowering and Cast insertion. Signed-off-by: Gwena Cunha <4861122+gcunhase@users.noreply.github.com>
Keep the NVFP4 ONNX exporter regression with the existing CPU Torch ONNX export coverage instead of a standalone test file. Signed-off-by: Gwena Cunha <4861122+gcunhase@users.noreply.github.com>
Use a protobuf-only graph node topological sort so NVFP4 post-processing preserves BF16 ONNX metadata while still returning producer-before-consumer node order. Signed-off-by: Gwena Cunha <4861122+gcunhase@users.noreply.github.com>
Share the protobuf-only ONNX graph node sorter from modelopt.onnx.utils and document why it avoids GraphSurgeon dtype round-tripping for enum dtypes such as BF16. Signed-off-by: Gwena Cunha <4861122+gcunhase@users.noreply.github.com>
Cache Cast outputs per input and precision dtype so multiple NVFP4-lowered MatMul consumers reuse a single activation Cast instead of producing duplicate tensor names. Signed-off-by: Gwena Cunha <4861122+gcunhase@users.noreply.github.com>
Include implicit outer-scope tensor references from nested ONNX graph attributes when sorting graph nodes so control-flow nodes are ordered after captured tensor producers. Signed-off-by: Gwena Cunha <4861122+gcunhase@users.noreply.github.com>
Import ONNX through pytest.importorskip before using ONNX helper symbols so partial torch unit sessions skip this optional coverage instead of failing during collection. Signed-off-by: Gwena Cunha <4861122+gcunhase@users.noreply.github.com>
c71f294 to
1afe0de
Compare
What does this PR do?
Type of change: Bug fix
NVFP4QuantExporter.post_process()returns.DequantizeLinearandCastproducer nodes appear before theTranspose/MatMulnodes that consume them without round-tripping BF16 metadata through ONNX GraphSurgeon.Usage
Testing
mainbefore this patch.producer_after_consumer_edges=0,onnx.checker.check_model(converted)passes, and the repro exits withrc=0.python -m pytest tests/unit/torch/quantization/test_onnx_export_cpu.py::test_nvfp4_exported_onnx_is_topologically_sorted tests/unit/torch/quantization/test_onnx_export_cpu.py::test_nvfp4_shared_activation_reuses_cast -q.ruff format modelopt/onnx/export/nvfp4_exporter.py modelopt/onnx/utils.py tests/unit/torch/quantization/test_onnx_export_cpu.pyandruff check modelopt/onnx/export/nvfp4_exporter.py modelopt/onnx/utils.py tests/unit/torch/quantization/test_onnx_export_cpu.py.tests/gpu/torch/quantization/test_nvfp4_onnx_export.py::test_simple_linear[BFloat16]by avoiding an ONNX GraphSurgeon import/export round-trip in the fix path.Before your PR is "Ready for review"
Make sure you read and follow Contributor guidelines and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices (e.g. avoiding hardcoded
trust_remote_code=True,torch.load(..., weights_only=False),pickle, etc.).CONTRIBUTING.md: N/ASummary by CodeRabbit
Bug Fixes
Tests
TRT_FP4QDQ, verification with ONNX model checks, and cast reuse behavior.